-
Notifications
You must be signed in to change notification settings - Fork 578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tpetra: Implementation of CrsGraph::transferAndFillComplete #2556
Conversation
…nds) CrsGraph::transferAndFillComplete is new, independent code. However, there were some function name clashes in packCrsMatrix and packCrsGraph. So, I moved implementation details of each to a new namespace so that they could have consistent naming. The same goes for unpackAndCombineCrsMatrix and unpackAndCombineCrsGraph. Addresses: trilinos#2267
@mhoemmen and @csiefer2 , there is a pant load of lines of code in this PR, no one in their right mind would want to go through all of them! Here is a quick summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Using Repos:
Pull Request Author: tjfulle |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4:00:00. If a change to the Pull Request source branch occurs, the testing will be attempted again. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this is huge!!! Thanks @tjfulle ! :-D
There are a few risky includes in Tpetra_CrsGraph_def.hpp
, so please be sure to test with ETI both ON and OFF. See note. Otherwise, it's a lot of code and I haven't read it all, but mainly it's all implementation details.
@@ -60,6 +60,11 @@ | |||
#include "Tpetra_Details_getEntryOnHost.hpp" | |||
#include "Tpetra_Distributor.hpp" | |||
#include "Teuchos_SerialDenseMatrix.hpp" | |||
#include "Tpetra_Vector.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to test with ETI both ON and OFF, since these includes are a bit risky. (That is, there's a risk of a circular include, since CrsMatrix depends on both CrsGraph and MultiVector, but it may only manifest in an ETI OFF build.)
using Teuchos::TimeMonitor; | ||
#endif | ||
|
||
using LO = LocalOrdinal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these C++11-style using
declarations! :-D
string label; | ||
if(!params.is_null()) label = params->get("Timer Label", label); | ||
string prefix2 = string("Tpetra ")+ label + std::string(": CrsGraph TAFC "); | ||
RCP<TimeMonitor> MM = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For later, don't handle TimeMonitor by RCP; just treat them as the scope guards that they are. For now, this is fine.
} // domainTransfer != null | ||
|
||
|
||
// FIXME (mfh 15 May 2014) Wouldn't communication still be needed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this is a valid question. "Locally replicated" implies "not distributed." The source Map could be locally replicated, even though the target Map is not. I think it's OK to leave this alone for now, though.
essentially_equal(T a, T b) { | ||
typedef Kokkos::ArithTraits<T> KAT; | ||
const auto eps = KAT::eps(); | ||
return KAT::abs(a - b) <= ( (KAT::abs(a) > KAT::abs(b) ? KAT::abs(b) : KAT::abs(a)) * eps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact: this should also return false for a or b NaN, which is what you want :-) .
@mhoemmen looks like it failed with gcc 4.8, do you know how to check the actual failure? |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Using Repos:
Pull Request Author: tjfulle |
@tjfulle It's on the Dashboard (yesterday's -- for some reason the clock doesn't turn over at midnight). If you expand the "Pull Request Testing has FAILED" details arrow, you'll find the "Build Num" (207, in this case). Match that and the pull request number with the builds in the "Pull Request" section of the Dashboard. Here is the build error: https://testing-vm.sandia.gov/cdash/viewBuildError.php?buildid=3431248 |
@tjfulle It looks like maybe you mixed up LO and GO:
|
@tjfulle Wait a second, though -- why did this build work with GCC 4.9.3 but not with 4.8.4? @jwillenbring @trilinos/framework Not sure what's going on here -- does the 4.9.3 build disable |
@mhoemmen, I saw that too. I thought I was enabling |
@tjfulle wrote:
It should be on by default, unless you set |
@mhoemmen, when I tested locally, I just leave the default instantiations (but with OpenMP on). It's not immediately jumping out at me where |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4:00:00. If a change to the Pull Request source branch occurs, the testing will be attempted again. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Using Repos:
Pull Request Author: tjfulle |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
@mhoemmen , apparently, gcc 4.8.4 was unable to deduce the template types on the call to |
@tjfulle I think that's OK. Earlier GCC versions generally are worse at deducing template parameters from function arguments. If all the template parameters are right, the |
Since this commit trilinos does not compile for me anymore, even a very simple test program would not compile anymore:
Trilinos is configured with the command: The test program is compiled with:
My compiler version:
Same errors with earlier gcc versions. The beginning of the error message:
|
@finkandreas try with the latest |
Yes the develop builds correctly. I assumed that the master branch is the one, that should always compile, so I reported it. |
@finkandreas Pull requests are supposed to go through pull request testing, in addition to local testing. That doesn't mean General best practice is to trust |
CrsGraph::transferAndFillComplete is new, independent code. However, there were
some function name clashes in packCrsMatrix and packCrsGraph. So, I moved implementation
details of each to a new namespace so that they could have consistent naming. The
same goes for unpackAndCombineCrsMatrix and unpackAndCombineCrsGraph.
@trilinos/tpetra
Related Issues
How Has This Been Tested?
Tpetra tests pass with GCC 6.4, openmpi 3.0.0, openmp
Checklist